Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New store settings system #11139

Draft
wants to merge 100 commits into
base: master
Choose a base branch
from

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jul 19, 2024

Motivation

See the linked issues for details.

The most notable user-relevant bits are:

  • This cleans up the MountedSSHStore: decomposed into its orthogonal parts

  • This brings us pretty close to being able to then implement a JSON-based config.

    • Store query parameters can be JSON
    • Stores can entirely be specified via JSON objects, but this is not yet hooked up to anything.

Also behind the scenes have these benefits:

  1. The docs are moved out of the headers, good for less rebuilding when they changes
  2. Stores are always constructed from store configs
  3. Use JSON, avoid custom serializers

Context

Do not review by commits

  • contains commits from a different PR that are listed here because that PR was squashed
  • not much of useful separation anyway in this commit history

Part of #11106
Part of #10766

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

fzakaria and others added 30 commits July 15, 2024 11:10
Following what is outlined in NixOS#10766 refactor the uds-remote-store such
that the member variables (state) don't live in the store itself but in
the config object.

Additionally, the config object includes a new necessary constructor
that takes a scheme & authority.

Minor:
* code formatting
* cleanup of getting default path
* added some comments
Revert back to basic constructor in store-api.cc

Co-authored-by: John Ericson <[email protected]>
Hopefully this fixes the macOS link error. It's also good for
compilation time.
Not having these causes some issues with the new unit tests.
It is a property of the configuration of a store --- how a store URL is
parsed into a store config, not a store itself.

Progress towards NixOS#10766
@roberth
Copy link
Member

roberth commented Aug 28, 2024

We should really keep the template metaprogramming to a minimum! Again for readability, this is not great since looking at this code, the reader has no idea what F is supposed to do.

I believe it's quite valuable in this situation, and not all that confusing when documented properly.

For this we can make use of the language server. (not using one is cruel anyway)

Example:

diff --git a/src/libstore/binary-cache-store.hh b/src/libstore/binary-cache-store.hh
index af649aaab..a638248c5 100644
--- a/src/libstore/binary-cache-store.hh
+++ b/src/libstore/binary-cache-store.hh
@@ -13,6 +13,7 @@ namespace nix {
 
 struct NarInfo;
 
+/** @param T Either 'nix::config::JustValue' or `nix::config::SettingInfo` */
 template<template<typename> class F>
 struct BinaryCacheStoreConfigT
 {
diff --git a/src/libutil/config-abstract.hh b/src/libutil/config-abstract.hh
index 2c34ff50b..e7f71e700 100644
--- a/src/libutil/config-abstract.hh
+++ b/src/libutil/config-abstract.hh
@@ -5,6 +5,9 @@
 
 namespace nix::config {
 
+/**
+ * Just a value, no metadata. explain explain
+ */
 template<typename T>
 struct JustValue
 {

With docs in these positions, readers can easily navigate to the explanations in the two relevant types that instantiate the fields.

Note also that when hovering over a config.foo usage (this->config.foo), the LSP will show JustValue<bool> which gives a good intuition for what to expect from it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c api Nix as a C library with a stable interface documentation new-cli Relating to the "nix" command repl The Read Eval Print Loop, "nix repl" command and debugger store Issues and pull requests concerning the Nix store with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: To triage
Development

Successfully merging this pull request may close these issues.

5 participants